feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export #3004

xunilrj · 2022-08-04T10:17:00Z

Summary

This expands #2979 with two extra cases:

Now we recommend removing the whole catch declaration:

warning[js/noUnusedVariables]: This variable is unused.
   ┌─ noUnusedVariables.js:39:10
   │
39 │ } catch (e) {}
   │          -
Suggested fix: Remove this variable.
      | @@ -36,4 +36,4 @@
35 35 |   };
36 36 |   
37 37 |   try {
38    | - } catch (e) {}
   38 | + } catch {}

and we don´t generate false positives to interfaces and abstract methods on Typescript and exported functions.

Test Plan

cargo run -p rome_cli -- check crates/rome_js_analyze/tests/specs/js/noUnusedVariablesTypescript.ts

cloudflare-workers-and-pages · 2022-08-04T10:17:26Z

Deploying with Cloudflare Pages

Latest commit:	`1e50f87`
Status:	✅ Deploy successful!
Preview URL:	https://a66bd4b0.tools-8rn.pages.dev
Branch Preview URL:	https://feature-no-unused-variable-i.tools-8rn.pages.dev

View logs

ematipico

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

github-actions · 2022-08-04T10:22:28Z

Playground for commit a524444

xunilrj · 2022-08-04T10:44:04Z

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

Done. But now we strangely have a "noUnusedVariables" group. Not so sure I like that.

ematipico · 2022-08-04T11:37:30Z

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

Done. But now we strangely have a "noUnusedVariables" group. Not so sure I like that.

Ah, I am not sure how things are set up, but I was referring to something like this:
https://github.com/rome/tools/tree/main/crates/rome_js_analyze/tests/specs/js/noDeadCode

leops · 2022-08-05T16:01:02Z

crates/rome_js_analyze/src/semantic_analyzers/no_unused_variables/no_unused_variables.rs

@@ -71,6 +72,43 @@ declare_rule! {
    }
 }

+// It is ok in some Typescripts constructs for a parameter to be unused.
+fn is_typescript_unused_ok(binding: &JsIdentifierBinding) -> Option<()> {


Should this check be moved to the semantic model builder ? It could make sense to consider that identifiers in TypeScript declarations do not create a new binding in the semantic model

Yes. It will as an "is_used" query. Or something like that.

ematipico · 2022-08-08T11:16:56Z

@xunilrj could you please re-open the issue or create new issues to list what we are missing in order to make the rule stable?

ematipico · 2022-08-08T11:18:31Z

crates/rome_js_analyze/src/semantic_analyzers/no_unused_variables/no_unused_variables.rs

+
+            Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::Unspecified,


We should either choose Always or MaybeIncorrect. Using Unspecified means that its applicability is unknown, which is not the case. Since the rule is still unstable, we should choose MaybeIncorrect

ematipico · 2022-08-10T11:50:32Z

crates/rome_js_analyze/tests/specs/no_unused_variables/noUnusedVariables.js.snap

+
+# Diagnostics
+```
+warning[no_unused_variables/noUnusedVariables]: This variable is unused.


Out of curiosity, is this expected? The no_unused_variables/ part. (it should be js)

hum... I think this is part of the confusion with "groups". I will fix it.

ematipico

A suggestion: maybe we should not recommend this rule for the time being?

leops · 2022-08-10T14:18:52Z

A suggestion: maybe we should not recommend this rule for the time being?

This is actually a very interesting issue: our lint rules are unstable, and some of them have a number of false positive issues due to the complexity of statically analyzing JavaScript code.
On one hand it's tempting to remove these rules from the recommended set since it largely degrades the initial experience of people trying out the toolchain, even if we advertise the linter as unstable.
But people encountering these bugs also generates valuable feedback on bugs that we actually need to fix, and by disabling these rules by default I think we risk having these rules be indefinitely unstable because they're only actually enabled by a handful of users, and we do not have enough information on how the rule actually performs "in the wild" to decide that a rule is stable enough to be enabled by default.

ematipico · 2022-08-10T14:47:13Z

Fortunately people can turn off rules via configuration now, so there's a good workaround in case of errors, and people won't be blocked. Less pressure for fixes. So yeah, I think it's worth turn them on and fix possible errors.

…catch, typescript and export (rome#3004) * improvements to noUnusedVariables to consider catch, typescript and export.

xunilrj requested a review from leops as a code owner August 4, 2022 10:17

xunilrj temporarily deployed to aws August 4, 2022 10:17 Inactive

ematipico reviewed Aug 4, 2022

View reviewed changes

xunilrj requested a review from a team August 4, 2022 10:43

xunilrj temporarily deployed to aws August 4, 2022 10:43 Inactive

xunilrj force-pushed the feature/no-unused-variable-improvements-catch-and-ts branch from ac87f6d to a20d3e4 Compare August 5, 2022 08:08

xunilrj temporarily deployed to aws August 5, 2022 08:08 Inactive

leops reviewed Aug 5, 2022

View reviewed changes

ematipico reviewed Aug 8, 2022

View reviewed changes

xunilrj force-pushed the feature/no-unused-variable-improvements-catch-and-ts branch from a20d3e4 to c0f2624 Compare August 10, 2022 07:46

xunilrj temporarily deployed to aws August 10, 2022 07:46 Inactive

xunilrj changed the title ~~feat(rome_js_analyze): improvements to noUnusedVariables to consider catch and typescript~~ feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export Aug 10, 2022

xunilrj temporarily deployed to aws August 10, 2022 11:40 Inactive

ematipico reviewed Aug 10, 2022

View reviewed changes

xunilrj force-pushed the feature/no-unused-variable-improvements-catch-and-ts branch from df5fa3c to c6ce7e5 Compare August 11, 2022 14:23

xunilrj temporarily deployed to aws August 11, 2022 14:23 Inactive

ematipico approved these changes Aug 11, 2022

View reviewed changes

xunilrj added 4 commits August 11, 2022 17:32

improvements to noUnusedVariables to consider catch and typescript

f524832

more typescript test

495b248

moving linter to support ts

2557cae

fmt and clippy issues

f1ef134

xunilrj added 6 commits August 11, 2022 17:32

fix test name generation

0d6d393

incorporate is_exported into noUnusedVariables

2d8f695

applicability as maybeincorrect

4ae6963

correctly moving noUnusedVariables to js folder

e175047

fix cli snap test

7c15fd2

fixing rebase issue

1e50f87

xunilrj force-pushed the feature/no-unused-variable-improvements-catch-and-ts branch from c6ce7e5 to 1e50f87 Compare August 11, 2022 16:39

xunilrj temporarily deployed to aws August 11, 2022 16:40 Inactive

xunilrj merged commit b2f6b5a into main Aug 11, 2022

xunilrj deleted the feature/no-unused-variable-improvements-catch-and-ts branch August 11, 2022 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export #3004

feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export #3004

xunilrj commented Aug 4, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 4, 2022 •

edited

Loading

ematipico left a comment

github-actions bot commented Aug 4, 2022 •

edited

Loading

xunilrj commented Aug 4, 2022

ematipico commented Aug 4, 2022

leops Aug 5, 2022

xunilrj Aug 5, 2022

ematipico commented Aug 8, 2022

ematipico Aug 8, 2022 •

edited

Loading

xunilrj Aug 10, 2022

ematipico Aug 10, 2022

xunilrj Aug 10, 2022

xunilrj Aug 11, 2022

ematipico left a comment

leops commented Aug 10, 2022

ematipico commented Aug 10, 2022 •

edited

Loading

feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export #3004

feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export #3004

Conversation

xunilrj commented Aug 4, 2022 • edited Loading

Summary

Test Plan

cloudflare-workers-and-pages bot commented Aug 4, 2022 • edited Loading

Deploying with Cloudflare Pages

ematipico left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 4, 2022 • edited Loading

xunilrj commented Aug 4, 2022

ematipico commented Aug 4, 2022

leops Aug 5, 2022

Choose a reason for hiding this comment

xunilrj Aug 5, 2022

Choose a reason for hiding this comment

ematipico commented Aug 8, 2022

ematipico Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

xunilrj Aug 10, 2022

Choose a reason for hiding this comment

ematipico Aug 10, 2022

Choose a reason for hiding this comment

xunilrj Aug 10, 2022

Choose a reason for hiding this comment

xunilrj Aug 11, 2022

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

leops commented Aug 10, 2022

ematipico commented Aug 10, 2022 • edited Loading

xunilrj commented Aug 4, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 4, 2022 •

edited

Loading

github-actions bot commented Aug 4, 2022 •

edited

Loading

ematipico Aug 8, 2022 •

edited

Loading

ematipico commented Aug 10, 2022 •

edited

Loading